Skip to content

Fix for null pointer dereference in jmem_heap_free_block #2440

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Aug 1, 2018

Conversation

DanielBallaSZTE
Copy link

Fixes #2435.

JerryScript-DCO-1.0-Signed-off-by: Daniel Balla [email protected]

Copy link
Contributor

@LaszloLango LaszloLango left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Member

@rerobika rerobika left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add the reported issue as a test case.

@dominiakm
Copy link

Hi,
I'm the original poster of the issue you are fixing here.

While this change does prevent segfault, undefined behavior is still triggered at jerryscript/jerry-core/ecma/builtin-objects/typedarray/ecma-builtin-typedarray-prototype.c:598

This is because behavior of memcpy is undefined if the second argument is NULL.
This can be verified by using UBSan (undefined behavior sanitizer).
To do this, build jerryscript with
python tools/build.py --profile=es2015-subset --jerry-libc=off --lto=off --compile-flag="-fsanitize=undefined"

Running the example provided by me shows the following message:
jerryscript/jerry-core/ecma/builtin-objects/typedarray/ecma-builtin-typedarray-prototype.c:598:7: runtime error: null pointer passed as argument 2, which is declared to never be null /usr/include/x86_64-linux-gnu/bits/string3.h:53:10: runtime error: null pointer passed as argument 2, which is declared to never be null.

Copy link
Member

@zherczeg zherczeg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you explain how this related to a memcpy?

@dominiakm
Copy link

dominiakm commented Jul 31, 2018

The implementation of filter (from jerryscript/jerry-core/ecma/builtin-objects/typedarray/ecma-builtin-typedarray-prototype.c) calls memcpy (in line 598) with the second argument (pass_value_list_p) being NULL.

Current implementation (without this commit) in JMEM_FINALIZE_LOCAL_ARRAY checks if var_name (pass_value_list_p in this case) is NULL. However, this check is optimized out, because of the call to memcpy. This is because gcc assumes, that that the second argument of memcpy is never NULL.

The default build for jerryscript enbales gcc optimisation -fdelete-null-pointer-checks, which removes any further checks for pass_value_list_p being NULL, for example the check in JMEM_FINALIZE_LOCAL_ARRAY.

@DanielBallaSZTE DanielBallaSZTE force-pushed the fix_jmem_array branch 2 times, most recently from 0971d4d to b6daa0b Compare July 31, 2018 14:06
@DanielBallaSZTE
Copy link
Author

I've added an early return to resolve this issue, and the test case as well.

@dominiakm
Copy link

This version no longer triggers undefined behavior.

{
return ret_value;
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need an extra newline.

else \
{ \
JERRY_ASSERT (var_name ## ___size == 0); \
} \
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change should be removed.

@DanielBallaSZTE DanielBallaSZTE force-pushed the fix_jmem_array branch 2 times, most recently from 0e58db4 to 675f06e Compare August 1, 2018 07:42
Copy link
Member

@zherczeg zherczeg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

if (len == 0)
{
return ret_value;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, why this is not before the define local array?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because in some cases we can not return with ECMA_VALUE_EMPTY

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so we return with a new typedarray?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

actually we return from a macro enclosed block, and that is not necessarily healthy.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes but we only return from there when the length is 0, therefore the enclosing macro would basically do nothing

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And yes, we have to give back a 0 length typedarray in that case

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem is we might change silently the macro and we have a hidden bug. Creating and returning with an empty array early should be easy.

Copy link
Author

@DanielBallaSZTE DanielBallaSZTE Aug 1, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that is definitely right, it would just mean a bit of a code duplication there.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The compiler should handle that. Since it is a single line, I would not even call it a duplication.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah you convinced me there, I'm updating the PR

Copy link
Member

@zherczeg zherczeg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@yichoi yichoi merged commit 8789784 into jerryscript-project:master Aug 1, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants